I feel we should try not to call sysdb for getting segments. It adds extra call to the backend for distributed chroma.

Seeing the current code, I see we are already calling sysdb.get_segments() from the manager, so you are simply moving that line here, and not adding extra calls. But i feel we can do better.

Do you think we should just call delete_segment() from delete_collection() ?
So we can add this snippet back -

   for s in self._manager.delete_segments(existing[0]["id"]):
       self._sysdb.delete_segment(s)

and do a no-op inside delete_segments() in db/impl/grpc/client.py
Will that fix the leak ?

@rohitcpbot,

using this snippet:

for s in self._manager.delete_segments(existing[0]["id"]): 
   self._sysdb.delete_segment(s)

Makes sense however we revert back to a non-atomic deletion of sysdb resources. In the above snippet we'd delete the segments separately from deleting the collection, which I wanted to avoid on purpose which is why I pulled the get of the segments here before the were atomically deleted as part of self._sysdb.delete_collection.

Why do you think that this would cause extra calls in the distributed backend?

@HammadB, talked with @rohitcpbot and he mentioned that this should be noop, is this fine or should I revert back to the older version with distributed sysdb query?

@rohitcpbot, this is the actual change as we discussed. rest is just black formatting changes.

Thanks @tazarov.
If possible leave a note with the following comment or similar -
"""
This call will delete segment related data that is stored locally and cannot be part of the atomic SQL transaction.
It is a NoOp for the distributed sysdb implementation.
Omitting this call will lead to leak of segment related resources.
"""

Can you answer something for me - If the process crashes immediately after self._manager.delete_segments(collection_id=existing[0].id)
Then the actual entries in SQL are not deleted, which means the collection is not deleted.
Now if user issues a Get or Query, will the local manager work correctly ?

If the fix to make local manager work in the above failure scenario is non trivial then we could leave a note here, and take it up as a separate task. But it will be good to know the state of the Db with above change.

The same scenario would had to be thought through even with your earlier changes of doing the local manager delete after the sysdb delete... where the sql could have gone through but the local manager did not because of a crash.. leading to a leak.

@rohitcpbot, local manager has two segments for each collection:

• sqlite - this will actually delete the segment from segments -

  chroma/chromadb/segment/impl/metadata/sqlite.py

  Line 595 in d50a942

  def delete(self) -> None:

• hnsw - it will delete the directory where the the HNSW is stored but will not delete the segment from segments dir

So here is a diagram to explain the point of failure:

The main problem as I see it in the current impl (with possible solutions):

I think the only foolproof way to remove it all is possibly to wrap it all in a single transaction all the way from segment. Then if the physical dir removal fails we'll rollback the whole sqlite transaction.

As a side note, on Windows deleting the segment dir right after closing file handles frequently fails.